Query endpoint #1643

anik120 · 2025-01-21T20:31:20Z

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

Signed-off-by: Joe Lanford <[email protected]>

netlify · 2025-01-21T20:31:38Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`478c152`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6791621d326c580008fff285
😎 Deploy Preview	https://deploy-preview-1643--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2025-01-21T20:51:52Z

Codecov Report

Attention: Patch coverage is 22.13115% with 285 lines in your changes missing coverage. Please review.

Project coverage is 63.97%. Comparing base (a19f91a) to head (478c152).

Files with missing lines	Patch %	Lines
catalogd/internal/storage/localdir.go	43.31%	96 Missing and 10 partials ⚠️
catalogd/internal/storage/index.go	0.00%	92 Missing ⚠️
catalogd/internal/storage/multireadseeker.go	0.00%	78 Missing ⚠️
catalogd/cmd/catalogd/main.go	0.00%	6 Missing ⚠️
catalogd/internal/serverutil/serverutil.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1643      +/-   ##
==========================================
- Coverage   67.42%   63.97%   -3.46%     
==========================================
  Files          55       57       +2     
  Lines        4632     4957     +325     
==========================================
+ Hits         3123     3171      +48     
- Misses       1284     1554     +270     
- Partials      225      232       +7

Flag	Coverage Δ
e2e	`53.21% <ø> (-0.09%)`	⬇️
unit	`51.84% <22.13%> (-2.61%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joelanford · 2025-01-21T21:03:29Z

catalogd/internal/storage/multireadseeker.go

@@ -0,0 +1,118 @@
+package storage


Oh, we should discuss this. I added this because the http package documentation recommended using http.ServeContent over io.Copy for a number of reasons, and http.ServeContent needs an io.ReadSeeker. The main benefit for us would be support for the Range http headers.

I found this multiReadSeaker implementation in a Golang issue proposing that it be added to the standard library, so there may be license concerns. We may need to implement our own or look into implementing the range functionality another way.

joelanford · 2025-01-21T22:15:36Z

catalogd/internal/serverutil/serverutil.go

+			BaseContext: func(_ net.Listener) context.Context {
+				return log.IntoContext(context.Background(), mgr.GetLogger().WithName("http.catalogs"))
+			},


I think we can either drop this whole BaseContext or alternately, simply return l

Either way, I don't think the code uses the logger from the context. This was a holdover from the way that the original logging middleware worked.

joelanford · 2025-01-21T22:23:24Z

catalogd/internal/serverutil/serverutil_tests.go

+// },
+
+// by default the compressor will only trigger for files larger than 1400 bytes
+const testCompressableJSON = `{


If all this is stuff is to test the gzip handler in isolation, I think we can simplify.

If the goal is to test the entire stack of handlers, I figure there's a better way to organize and test to reduce duplication and fragility. For example, if we make the CatalogServerConfig accept an arbitrary http.Handler instead of a storage.Instance, then we can:

test the storage.Instance handler in isolation

test the serverutil setup with dummy handlers that are easier to manipulate for test purposes.

Or, the other relevant question is, do we need to test the gzip handler explicitly in our unit tests? We're importing "github.com/klauspost/compress/gzhttp" to provided compressed content via our server. Are unit tests that say "tests if the library is doing its job" really that useful?

joelanford · 2025-01-21T22:29:12Z

catalogd/internal/serverutil/serverutil_tests.go

+// {
+// 	name: "Ignores accept-encoding for the path /catalogs/test-catalog/api/v1/all with size < 1400 bytes",
+// 	setupStore: func() error {
+// 		return writeFile(filepath.Join(store.RootDir, "test-catalog2", "catalog.jsonl"), []byte(`{"foo":"bar"}`), 0600)


I know existing tests violate this rule, but one thing I think I would fix is to avoid writing to the underlying filesystem, because it is pretty fragile. I think we can probably test as a black box, something like:

Check ContentExists(foo) is false

Store(foo)

Query various endpoints with positive and negative tests for foo, bar catalogs, but also just different path prefixes, etc.

Check ContentExists(foo) is true

Delete(foo)

Query foo endpoints and expect 404

Check ContentExists(foo) is false.

+1, I've added this test in localdir_test.go, and avoided writing directly to the file system (using store.Store instead)

azych · 2025-01-22T08:47:28Z

What's the relation of this PR to #1642?
This one seems to include two additional commits, with the first 4 being the same.

Which PR is a duplicate and which should be reviewed?
If this is the one to review (and also not a Draft), can you add a description of the main changes this introduces?

anik120 · 2025-01-22T21:31:51Z

What's the relation of this PR to #1642? This one seems to include two additional commits, with the first 4 being the same.

Which PR is a duplicate and which should be reviewed? If this is the one to review (and also not a Draft), can you add a description of the main changes this introduces?

@azych apologies for the confusion. You're just seeing the result of rapid experimentation. This is the one to review, but it's still a WIP. I've converted this PR to a draft.

joelanford · 2025-01-22T22:10:53Z

catalogd/internal/storage/localdir_test.go

+		// 		}
+
+		// 		// Verify index file was created
+		// 		indexPath := filepath.Join(s.RootDir, "test-catalog", "index.json")


I would avoid the hardcoded path because it is pretty fragile. Maybe use:

indexPath := catalogIndexFilePath(s.catalogDir("test-catalog"))

joelanford added 4 commits January 21, 2025 14:44

add a new feature-gated api/v1/query endpoint

04a1984

Signed-off-by: Joe Lanford <[email protected]>

add singleflight for shared index access for concurrent requests

0ad0cec

Signed-off-by: Joe Lanford <[email protected]>

a few improvements and optimizations

f4ed498

Signed-off-by: Joe Lanford <[email protected]>

another round of refactoring improvement

5cc755d

Signed-off-by: Joe Lanford <[email protected]>

anik120 requested a review from a team as a code owner January 21, 2025 20:31

anik120 force-pushed the query-enpoint branch from 1a96032 to a27fada Compare January 21, 2025 20:34

joelanford reviewed Jan 21, 2025

View reviewed changes

convert ginkgo localDir tests

8285ca7

anik120 force-pushed the query-enpoint branch 2 times, most recently from 17e0f9a to 729645c Compare January 21, 2025 21:57

joelanford reviewed Jan 21, 2025

View reviewed changes

Clean up tests

478c152

anik120 force-pushed the query-enpoint branch from 729645c to 478c152 Compare January 22, 2025 21:24

anik120 marked this pull request as draft January 22, 2025 21:30

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2025

anik120 mentioned this pull request Jan 22, 2025

Query api #1642

Closed

4 tasks

joelanford reviewed Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query endpoint #1643

Query endpoint #1643

anik120 commented Jan 21, 2025

netlify bot commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

joelanford Jan 21, 2025

joelanford Jan 21, 2025

joelanford Jan 21, 2025

joelanford Jan 21, 2025

anik120 Jan 22, 2025

joelanford Jan 21, 2025 •

edited

Loading

anik120 Jan 22, 2025

azych commented Jan 22, 2025

anik120 commented Jan 22, 2025

joelanford Jan 22, 2025

Query endpoint #1643

Are you sure you want to change the base?

Query endpoint #1643

Conversation

anik120 commented Jan 21, 2025

Description

Reviewer Checklist

netlify bot commented Jan 21, 2025 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

joelanford Jan 21, 2025

Choose a reason for hiding this comment

joelanford Jan 21, 2025

Choose a reason for hiding this comment

joelanford Jan 21, 2025

Choose a reason for hiding this comment

joelanford Jan 21, 2025

Choose a reason for hiding this comment

anik120 Jan 22, 2025

Choose a reason for hiding this comment

joelanford Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

anik120 Jan 22, 2025

Choose a reason for hiding this comment

azych commented Jan 22, 2025

anik120 commented Jan 22, 2025

joelanford Jan 22, 2025

Choose a reason for hiding this comment

netlify bot commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

joelanford Jan 21, 2025 •

edited

Loading